OCPCLOUD-2010: Add CloudControllerManagerStatus to External platform status #1434

adriengentil · 2023-04-05T13:10:50Z

Add the CloudControllerManagerStatus to the ExternalPlatformStatus struct.
The new field is added behind TechPreviewNoUpgrade feature set.

This setting will be set at installation time.

Enhancement doc update PR (OCPCLOUD-2011: Update implementation history of External platform type EP enhancements#1378)

openshift-ci-robot · 2023-04-05T13:10:54Z

@adriengentil: This pull request references OCPCLOUD-2010 which is a valid jira issue.

In response to this:

Revert PR #1409 in order to support
external platform type.

Enhancement doc update PR ()

This reverts commit e8d6cd0.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2023-04-05T13:11:26Z

Hello @adriengentil! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

openshift-ci · 2023-04-05T13:11:27Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci-robot · 2023-04-05T13:23:51Z

@adriengentil: This pull request references OCPCLOUD-2010 which is a valid jira issue.

In response to this:

Revert PR #1409 in order to support
external platform type.

Enhancement doc update PR (OCPCLOUD-2011: Update implementation history of External platform type EP enhancements#1378)

This reverts commit e8d6cd0.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

JoelSpeed · 2023-04-05T13:44:13Z

Revert so not much to review here, but, do we have an implementation PR that can be reviewed alongside this API change?

adriengentil · 2023-04-05T15:42:25Z

Revert so not much to review here, but, do we have an implementation PR that can be reviewed alongside this API change?

I'm currently implementing the other part in library-go, I'll link it here once I have a draft.

…xternal This change enables the external platform when external platform type is set and when cloud controller manager mode is set to External. Since cloud controller manager mode is controlled from the `PlatformSpec` struct, I had to change the signature of `IsCloudProviderExternal` to take the `Infrastructure` struct in parameter. PR on API side: openshift/api#1434

elmiko

/lgtm

@JoelSpeed ptal

…xternal This change enables the external platform when external platform type is set and when cloud controller manager mode is set to External. Since cloud controller manager mode is controlled from the `PlatformSpec` struct, I had to change the signature of `IsCloudProviderExternal` to take the `Infrastructure` struct in parameter. PR on API side: openshift/api#1434

JoelSpeed · 2023-04-06T14:36:02Z

config/v1/types_infrastructure.go

+	// and no extra initialization from the cloud controller manager is expected.
+	// +kubebuilder:validation:Enum="";External;None
+	// +optional
+	State CloudControllerManagerState `json:"state"`


Do we allow this to be changed post install? Are there any transitions we would like to prevent, eg, should the External not be allowed to transition back to None?

we have never discussed being able to transit back from CCMs to non-CCMs. given that this field is intended for deployments where there most likely won't be an in-tree provider, i don't think we will allow it to be changed after install.

i'm trying to think about what that would mean, and essentially it would be switching a cluster from using CCMs to using no cloud controllers. which sounds like it might work in theory, but i'm not sure about the transitive effects and if that is supportable for us.

Well that's an interesting point, is it us supporting that change, or is it up to the people integrating into external?
Effectively this drives whether new nodes are tainted or not, so, transitioning away from it, isn't the worst thing in the world. But I don't know what the implications would be for things like load balancers if the CCM were suddenly removed.

We can add validation to say that it cannot be changed away from External once it is set to external, but are we sure that's the right thing to do?

Another thing we can do, is add a copy of this to the status, and then have some control determine if the change is acceptable or not, and if it is, update the status, if it's not, it doesn't. Consumers would only read from the status.

That said, can we confidently say that there is a blanket policy that will be acceptable for all external platforms?

Should we be putting this only in the status to signal that it should only be configured at install time, or did we want to support upgrading and installing CCMs at a later point? (I think we did, want to check that)

Joel and i discussed this in our standup today, we have some questions about whether this field should be in the spec or status of the infra object. i'd like to get a few more opinions about how we intend this feature to be used, and if it is appropriate for users to change it after installation.

adriengentil · 2023-04-21T15:35:20Z

I updated the PR to:

make spec.platformSpec.external.cloudControllerManager.state immutable (the only possible transition is from "" to None, which somehow makes sense?)
mirror spec.platformSpec.external.cloudControllerManager.state to status.platformStatus.external.cloudControllerManager.state, I guess we don't need to define any validation here as we want to reconcile the value from the spec

If it looks fine to you, I'll update the enhancement and the library-go PR.

Last question, do you have some pointers/hints to implement the reconciliation loop between both values?

…xternal This change enables the external platform when external platform type is set and when cloud controller manager mode is set to External. PR on API side: openshift/api#1434

elmiko

thanks for the patience @adriengentil , this is looking how i would expect now.

/lgtm

@JoelSpeed ptal

elmiko · 2023-04-24T14:16:08Z

Last question, do you have some pointers/hints to implement the reconciliation loop between both values?

good question, i'll have to dig a little or maybe ask Joel at our standup today.

elmiko · 2023-04-24T14:19:12Z

i do see some the values updated in the installer, i'm guessing we will need to do this when adding support for external platform there, but i'm not sure if this is definitive.

https://github.com/openshift/installer/blob/master/pkg/asset/manifests/infrastructure.go

adriengentil · 2023-06-12T16:53:20Z

Coming back to this thought, all we actually care about is that it doesn't go from empty to External, so the transition rule really should either allow it to stay as is, or allow it to transition not to External
// +kubebuilder:validation:XValidation:rule="(has(self.state) == has(oldSelf.state)) || (!has(oldSelf.state) && self.state != \"External\"")

I updated the CEL as you suggested, thanks!

JoelSpeed · 2023-06-12T23:25:42Z

/lgtm

/hold, we have a scheduled meeting to discuss this, lets merge during the meeting assuming other pre-requisites are all in place

…xternal This change enables the external platform when external platform type is set and when cloud controller manager mode is set to External. PR on API side: openshift/api#1434

Let CI run with api pointing to openshift/api#1434

…port Revendor openshift/api and openshift/library-go in order to support the external platform type. library-go PR: openshift/library-go#1496 API PR: openshift/api#1434

elmiko

/lgtm

/hold

@adriengentil feel free to remove the hold once openshift/cluster-config-operator#306 is passing the e2e-aws-ovn-techpreview test, or we can determine it is not failing due to this change.

elmiko · 2023-06-13T13:01:27Z

config/v1/feature_gates.go

@@ -83,6 +83,16 @@ var (
 		OwningProduct:       ocpSpecific,
 	}

+	FeatureGateExternalCloudProviderExternal = FeatureGateName("ExternalCloudProviderExternal")


agree that the stutter isn't great, but since it follows the others i think it can be acceptable

openshift-ci · 2023-06-13T13:35:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adriengentil, elmiko, JoelSpeed

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JoelSpeed]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

adriengentil · 2023-06-13T14:51:26Z

/unhold

all tests passed in openshift/cluster-config-operator#306

openshift-ci · 2023-06-13T15:12:24Z

@adriengentil: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

…xternal This change enables the external platform when external platform type is set, when cloud controller manager mode is set to External, and when the feature `FeatureGateExternalCloudProviderExternal` is enabled. PR on API side: openshift/api#1434

Re-vendor openshift/api in order to support the external platform. API PR: openshift/api#1434

…xternal This change enables the external platform when external platform type is set, when cloud controller manager mode is set to External, and when the feature `FeatureGateExternalCloudProviderExternal` is enabled. PR on API side: openshift/api#1434

…port Revendor openshift/api and openshift/library-go in order to support the external platform type. library-go PR: openshift/library-go#1496 API PR: openshift/api#1434

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Apr 5, 2023

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 5, 2023

openshift-ci bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 5, 2023

adriengentil marked this pull request as ready for review April 5, 2023 13:25

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 5, 2023

openshift-ci bot requested review from deads2k and JoelSpeed April 5, 2023 13:26

adriengentil mentioned this pull request Apr 6, 2023

OCPCLOUD-2010: Add external platform type support in IsCloudProviderExternal openshift/library-go#1496

Merged

elmiko reviewed Apr 6, 2023

View reviewed changes

openshift-ci bot assigned elmiko Apr 6, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 6, 2023

JoelSpeed reviewed Apr 6, 2023

View reviewed changes

openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 21, 2023

adriengentil force-pushed the platform-external branch from 8b8e205 to 55ae87c Compare April 21, 2023 15:25

elmiko reviewed Apr 24, 2023

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 24, 2023

adriengentil added 3 commits June 12, 2023 13:44

Add field validations and tests

3361940

Set CloudControllerManagerStatus behind TechPreviewNoUpgrade feature set

99f7e5b

update CEL to be more accurate

db0028c

adriengentil force-pushed the platform-external branch from 372de2e to db0028c Compare June 12, 2023 16:45

openshift-ci bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 12, 2023

openshift-ci bot assigned JoelSpeed Jun 12, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 12, 2023

adriengentil added a commit to adriengentil/cluster-config-operator that referenced this pull request Jun 13, 2023

WIP: Test external platform api change

19d072b

Let CI run with api pointing to openshift/api#1434

adriengentil mentioned this pull request Jun 13, 2023

OCPCLOUD-2010: Re-vendor api and library-go for external platform support openshift/machine-config-operator#3745

Merged

elmiko reviewed Jun 13, 2023

View reviewed changes

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 13, 2023

openshift-merge-robot merged commit ba04973 into openshift:master Jun 13, 2023

adriengentil added a commit to adriengentil/cluster-config-operator that referenced this pull request Jun 13, 2023

OCPCLOUD-2010: Re-vendor api to support external platform

e4ea2e1

Re-vendor openshift/api in order to support the external platform. API PR: openshift/api#1434

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPCLOUD-2010: Add CloudControllerManagerStatus to External platform status #1434

OCPCLOUD-2010: Add CloudControllerManagerStatus to External platform status #1434

adriengentil commented Apr 5, 2023 •

edited

Loading

openshift-ci-robot commented Apr 5, 2023 •

edited by openshift-ci bot

Loading

openshift-ci bot commented Apr 5, 2023

openshift-ci bot commented Apr 5, 2023

openshift-ci-robot commented Apr 5, 2023 •

edited by openshift-ci bot

Loading

JoelSpeed commented Apr 5, 2023

adriengentil commented Apr 5, 2023

elmiko left a comment

JoelSpeed Apr 6, 2023

elmiko Apr 6, 2023

JoelSpeed Apr 6, 2023

elmiko Apr 6, 2023

adriengentil commented Apr 21, 2023

elmiko left a comment

elmiko commented Apr 24, 2023

elmiko commented Apr 24, 2023

adriengentil commented Jun 12, 2023

JoelSpeed commented Jun 12, 2023

elmiko left a comment

elmiko Jun 13, 2023

openshift-ci bot commented Jun 13, 2023

adriengentil commented Jun 13, 2023

openshift-ci bot commented Jun 13, 2023

OCPCLOUD-2010: Add CloudControllerManagerStatus to External platform status #1434

OCPCLOUD-2010: Add CloudControllerManagerStatus to External platform status #1434

Conversation

adriengentil commented Apr 5, 2023 • edited Loading

openshift-ci-robot commented Apr 5, 2023 • edited by openshift-ci bot Loading

openshift-ci bot commented Apr 5, 2023

openshift-ci bot commented Apr 5, 2023

openshift-ci-robot commented Apr 5, 2023 • edited by openshift-ci bot Loading

JoelSpeed commented Apr 5, 2023

adriengentil commented Apr 5, 2023

elmiko left a comment

Choose a reason for hiding this comment

JoelSpeed Apr 6, 2023

Choose a reason for hiding this comment

elmiko Apr 6, 2023

Choose a reason for hiding this comment

JoelSpeed Apr 6, 2023

Choose a reason for hiding this comment

elmiko Apr 6, 2023

Choose a reason for hiding this comment

adriengentil commented Apr 21, 2023

elmiko left a comment

Choose a reason for hiding this comment

elmiko commented Apr 24, 2023

elmiko commented Apr 24, 2023

adriengentil commented Jun 12, 2023

JoelSpeed commented Jun 12, 2023

elmiko left a comment

Choose a reason for hiding this comment

elmiko Jun 13, 2023

Choose a reason for hiding this comment

openshift-ci bot commented Jun 13, 2023

adriengentil commented Jun 13, 2023

openshift-ci bot commented Jun 13, 2023

adriengentil commented Apr 5, 2023 •

edited

Loading

openshift-ci-robot commented Apr 5, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Apr 5, 2023 •

edited by openshift-ci bot

Loading